Refactor: Improve Edge process management and JSON handling in patch …#2
Refactor: Improve Edge process management and JSON handling in patch …#2lifei6671 wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Edge patching logic to use generic JSON maps instead of fixed structs, ensuring that non-target fields are preserved during updates. It also introduces unit tests and modularizes process management and file I/O. Feedback highlights the need for atomic file writes to prevent corruption, improved helper process detection on macOS, and a fix for an incorrect error check when waiting for user input at the end of the execution.
| func writeJSONObject(path string, value map[string]any) error { | ||
| data, err := json.Marshal(value) | ||
| if err != nil { | ||
| return fmt.Errorf("marshal JSON file %s: %w", path, err) | ||
| } | ||
| if err := os.WriteFile(path, data, 0644); err != nil { | ||
| return fmt.Errorf("write JSON file %s: %w", path, err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This function has two issues:
- Non-atomic write: Writing directly to the target file using
os.WriteFileis risky. If the process is interrupted or the disk is full during the write, the browser profile file could be corrupted. It's safer to write to a temporary file and then rename it. - JSON formatting: The previous implementation used
json.MarshalIndentto maintain readability. Switching tojson.Marshalproduces minified JSON, which makes manual inspection of browser configuration files much harder.
func writeJSONObject(path string, value map[string]any) error {
data, err := json.MarshalIndent(value, "", " ")
if err != nil {
return fmt.Errorf("marshal JSON file %s: %w", path, err)
}
tmpPath := path + ".tmp"
if err := os.WriteFile(tmpPath, data, 0644); err != nil {
return fmt.Errorf("write temp JSON file %s: %w", tmpPath, err)
}
if err := os.Rename(tmpPath, path); err != nil {
_ = os.Remove(tmpPath)
return fmt.Errorf("rename JSON file %s: %w", path, err)
}
return nil
}| // 如果父进程同名，通常说明当前是子进程或 helper，直接跳过。 | ||
| if ppid, _ := p.Ppid(); ppid > 0 { | ||
| if parent, err := process.NewProcess(ppid); err == nil { | ||
| if parentName, err := parent.Name(); err == nil && parentName == name { |
There was a problem hiding this comment.
The current heuristic parentName == name to skip helper processes works on Windows and Linux where helpers share the same executable name (msedge). However, on macOS, the main process is typically named Microsoft Edge while helpers are named Microsoft Edge Helper. This check will fail to identify helpers on macOS, causing them to be killed individually. Using isEdgeProcessName for the parent name is a more robust approach across all platforms.
| if parentName, err := parent.Name(); err == nil && parentName == name { | |
| if parentName, err := parent.Name(); err == nil && isEdgeProcessName(runtime.GOOS, parentName) { |
| if _, err := fmt.Scanln(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Warning: failed to read input: %v\n", err) | ||
| } |
There was a problem hiding this comment.
fmt.Scanln() without arguments returns an unexpected newline error when the user presses Enter (as it expects to scan at least one item). This causes the warning message to be printed every time the program exits normally. You should either ignore the error or use a different method to wait for the Enter key.
fmt.Scanln()1 participant
…functions